Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a few issues in get_object_info_execute in AuthorityAggregator #473

Merged
merged 1 commit into from
Feb 18, 2022

Conversation

lxfind
Copy link
Contributor

@lxfind lxfind commented Feb 17, 2022

This PR fixes 2 issues along with some cleanups in the function:

  1. When iterating all mutated objects, it looked at mutated and created but that doesn't include the gas object. Using the proper API for it. On a side note, maybe we should have mutated to include the gas object. Otherwise it's confusing. In anyway using a simpler API call make this easier.
  2. The integrity of the latest object is not guaranteed. Added checks in SafeClient for it.

@oxade
Copy link
Contributor

oxade commented Feb 17, 2022

mutated used to include gas object.
What was the reason it was removed to begin with?

@lxfind
Copy link
Contributor Author

lxfind commented Feb 17, 2022

mutated used to include gas object. What was the reason it was remove to begin with?

We added a dedicated gas_object field to OrderEffects so that it's easier to check on it. There we removed it from mutated. But I do think it's more clear if mutated contains it. I can do that in a separate PR. This PR still applies though.

@lxfind lxfind force-pushed the cleanup-get_object_info branch 2 times, most recently from c27eae1 to 40d9c96 Compare February 18, 2022 04:06
Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add at least a TODO on the line that says

// Report a byzantine fault here

And then ... just doesn't?

@lxfind lxfind force-pushed the cleanup-get_object_info branch 2 times, most recently from 59e8b6a to 9e698e1 Compare February 18, 2022 16:29
@lxfind lxfind requested a review from huitseeker February 18, 2022 16:54
@lxfind
Copy link
Contributor Author

lxfind commented Feb 18, 2022

Added TODOs

Copy link
Collaborator

@gdanezis gdanezis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good fixes! But not sure what is left to validate (see comment).

@gdanezis
Copy link
Collaborator

Also, lets make a note: the code here relies on collecting lots of effects to check correct execution. It would be far simpler to provide object inputs and re-executing on the client instead.

@lxfind
Copy link
Contributor Author

lxfind commented Feb 18, 2022

How can it be simpler? We need to have the authorities process the certificate anyway, right?

Copy link
Collaborator

@sblackshear sblackshear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test

@lxfind lxfind merged commit 4a2385f into main Feb 18, 2022
@lxfind lxfind deleted the cleanup-get_object_info branch February 18, 2022 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants